-
Notifications
You must be signed in to change notification settings - Fork 293
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure Content-Length
set on requests with known length bodies
#981
Conversation
c68ab49
to
0cc7836
Compare
if (method != kj::HttpMethod::HEAD && method != kj::HttpMethod::GET && | ||
headers.get(kj::HttpHeaderId::CONTENT_LENGTH) == nullptr && | ||
headers.get(kj::HttpHeaderId::TRANSFER_ENCODING) == nullptr) { | ||
KJ_IF_MAYBE(l, requestBody.tryGetLength()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the length isn't known, should we set Transfer-Encoding: chunked
to emulate what an actual HTTP connection would do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hehe, sure! 🙂
src/workerd/api/global-scope.c++
Outdated
// If the request doesn't specify "Content-Length" or "Transfer-Encoding", set "Content-Length" | ||
// to the body length if it's known. This ensures handlers for worker-to-worker requests can | ||
// access known body lengths if they're set, without buffering bodies. | ||
if (method != kj::HttpMethod::HEAD && method != kj::HttpMethod::GET && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GET can have a body. Instead of checking the method, maybe what you want is body != nullptr
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GET can have a body?! Fun! Updated 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... it's quite disturbing, but some people use it and we had to add support for it in the early days of Workers.
0cc7836
to
4ace1e4
Compare
This ensures requests with known length bodies from service bindings, or other internal requests from bindings (e.g. `R2Bucket#put()`), share that known length with the handler.
4ace1e4
to
6d25855
Compare
Hey! 👋 This PR ensures requests with known length bodies from service bindings, or other internal requests from bindings (e.g.
R2Bucket#put()
), share that known length with the handler.I'm not sure whether this change needs a compatibility flag. I'm leaning towards no, as we're only adding a header if
Content-Length
andTransfer-Encoding
are missing. Although as highlighted by the new test, if aFixedLengthStream
'sreadable
was used as a body, previouslyContent-Length
wouldn't have been set. This is a user facing change, but I don't think it's breaking.